fix: frontend malformed uri crash#31921
Conversation
|
Didn't dig into this, but this seems like a bug that should be fixed in the library, not as a workaround in Z2M? |
|
I think this is more of a contract mismatch than a purely library-side bug. Z2M is using express-static-gzip successfully on the normal path with raw Node HTTP objects plus a few express-like fields, but the malformed-URI branch calls res.status(...).send(...), which is not available there. So the failing part is specifically the error path under Z2M’s current integration. Since this took a fair bit of digging to pin down, I thought it worth contributing the small, tested fix here rather than keeping it as a local patch. That said, an upstream safeguard in the library would be better indeed. |
|
Can you take a look at https://github.com/tkoenig89/express-static-gzip see if you can find the offending location(s)? |
|
Already on it. Probably. I opened the issue upstream and got a patch ready. |
|
That was pleasantly quick: the library-side fix is now merged upstream and released as 3.0.1. Z2M will still only benefit once that lands in a release and is bumped here, so I’m happy with whichever route you prefer: keep this local guard, or rely on the upstream fix once it is consumable. |
|
Bumped in #31924, thanks! |
Fixes #31920
Summary
This change stops Zigbee2MQTT from crashing when the FE receives a malformed percent-encoded request path.
The symptom is:
The correct behaviour here is rather boring: send 400 and move on, and, more importantly, don't crash.
Root cause
Frontend.onRequest()rewritesrequest.url/request.pathand then passes raw NodeIncomingMessage/ServerResponseobjects intoexpress-static-gzip.That works fine for normal requests. It goes sideways when
req.pathcontains invalid percent-encoding.Inside
express-static-gzip, the middleware does roughly this:decodeURIComponent(req.path)res.status(400).send(e.message)The snag is that
reshere is not an Express response. It is a plain NodeServerResponse, sores.statusdoes not exist. Instead of returning a 400, the process crashes.How this turned up
The field report that led me here came from an elderly but previously functional setup:
2.10.0That context sent me off chasing adapter gremlins for rather longer than I care to admit. The bridge had already been opened, the LEDs had been civilised with resistor surgery, a new USB socket had been soldered on, the PSU and cables had been swapped, and various other acts of honest suspicion had been committed. It still happened, like clockwork. After all that, the culprit turned out to be a malformed FE request path - and this took down all my zigbee devices for half a minute, every 5 minutes, in the HA endpoint.
To be clear, the crash itself is not an
ezspbug. The adapter and hardware were simply the scenery around the crime.In the environment that exposed this, an unrelated local service lobbed malformed HTTP requests at the frontend port due to a shared host/network layout. Daft, yes, but a malformed URL should still earn a 400, not a dead bridge.
What this patch does
Before handing the request to
express-static-gzip, validaterequest.pathwithdecodeURIComponent().If decoding fails:
400Content-Type: text/plain; charset=utf-8If decoding succeeds:
This keeps the fix small and local. One bad URL no longer gets to kill the whole process.
Validation
Added a regression test for a malformed path:
The new test verifies that Zigbee2MQTT:
400I also ran the frontend test suite on this branch.
Beyond that, I validated the built patch mildly. This wasn't a full coordinator-backed integration test, but it does exercise the real HTTP and WebSocket paths in-container.
Observed results:
GET /returns200200device_iconsfile returns200device_iconsfile returns normal 404GET /%E0%A4%Areturns400and crashesGET /device_icons/%E0%A4%Areturns400/apisucceedsbase_url: /z2m:GET /z2mredirects to/z2m/GET /z2m/returns200/z2m/assets/...returns200/z2m/device_icons/...returns200/z2m/device_icons/...returns normal 404GET /z2m/%E0%A4%Areturns400/z2m/apisucceedsMost importantly, after the malformed requests the patched containers stayed up. For comparison, stock
2.10.0under the same harness still crashes on/%E0%A4%Awith the originalres.status is not a functionstack.Notes
This is in Zigbee2MQTT's frontend server wrapper rather than in the frontend asset package itself. The middleware's assumption is understandable in an Express app; it is merely wrong in this particular bit of plumbing.